Skip to content

Conversation

@smoke
Copy link
Contributor

@smoke smoke commented Oct 1, 2024

  • Using WrapperConfiguration.flushTimeout() for consistency with TracingRequestStreamWrapper
  • Respects OTEL_LAMBDA_FLUSH_TIMEOUT env var
  • Breaking change: inherited from OTEL_LAMBDA_FLUSH_TIMEOUT_DEFAULT the flush timeout is now 10 seconds, was 1 second

@smoke smoke requested a review from a team as a code owner October 1, 2024 20:37
}

OpenTelemetrySdkAccess.forceFlush(1, TimeUnit.SECONDS);
OpenTelemetrySdkAccess.forceFlush(flushTimeoutNanos, TimeUnit.NANOSECONDS);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
OpenTelemetrySdkAccess.forceFlush(flushTimeoutNanos, TimeUnit.NANOSECONDS);
OpenTelemetrySdkAccess.forceFlush(flushTimeout(), TimeUnit.SECONDS);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OTEL_LAMBDA_FLUSH_TIMEOUT_DEFAULT is 10 seconds.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, OTEL_LAMBDA_FLUSH_TIMEOUT_DEFAULT is 10, that is a braking change from 1 second.
How to add proper changelog?

@SuppressWarnings("unused")
public static class HandleRequestAdvice {

private final static long flushTimeoutNanos = flushTimeout().toNanos();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
private final static long flushTimeoutNanos = flushTimeout().toNanos();

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have intentionally matched the reference implementation from https://github.com/search?q=repo%3Aopen-telemetry%2Fopentelemetry-java-instrumentation+flushTimeout.toNanos&type=code

Made it static as it is unlikely to change within the runtime, but I can make it non-static and non-final attribute if that will help.

…TATION_AWS_LAMBDA_FLUSH_TIMEOUT`

Using `WrapperConfiguration.flushTimeout()` for consistency with `TracingRequestStreamWrapper`
@smoke smoke requested a review from heyams October 17, 2024 11:30
@smoke smoke force-pushed the fix/aws-lambda-java-instrumentation-force-flush-based-on-config branch from 90c8a46 to 757a154 Compare October 17, 2024 11:33
@SuppressWarnings("unused")
public static class HandleRequestAdvice {

private static final long flushTimeoutNanos = flushTimeout().toNanos();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@laurit
Copy link
Contributor

laurit commented Nov 7, 2024

superseded by #12576

@laurit laurit closed this Nov 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants